fix(ui): make the accordion trigger hover a state layer - #1137
Conversation
Replaces the label dim that landed with #1126. Reduced prominence is this kit's disabled vocabulary (opacity-50, including on this very trigger), so dimming on hover said unavailable and under-your-pointer on the same axis, and it lowered text contrast at the exact moment of interaction. A background state layer is what Material, Carbon, and Polaris specify for disclosure headers, and it is already the kit's own most-used hover treatment (dropdown items, tabs), so the trigger now uses the same hover:bg-accent pair as a ghost button. The -mx-2/px-2 inset keeps the label aligned with the panel content while the tint breathes around it, rounded-md keeps the tint from reading as a table row, and dropping the explicit text-foreground restores rest-colour inheritance, undoing the small behaviour change the dim needed. Verified hovered in a real browser in light and dark inside the gallery preview scope.
Review caught that width 100 percent plus symmetric negative margins is over-constrained, so the browser drops the end-side margin: the tint bled 8px past the left divider while stopping 8px short of the right one, and the chevron sat 16px in from where every other row ends. A summary styled as flex is a block-level box that already fills its containing block, so removing w-full lets both negative margins apply. Measured after: 8px overhang on both sides, label still aligned with the panel content. Also corrects the file's design-token doc line, which still declared the foreground token this change stopped using and omitted the two accent tokens it now does.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Reviewed the state-layer change with fresh eyes and found two real things, both now fixed.
The substantive one: w-full plus symmetric negative margins is over-constrained under border-box sizing, so the browser silently drops the end-side margin. The tint bled 8px past the left divider while stopping 8px short of the right, and the chevron sat 16px in from where every other row ends, with the asymmetry flipping in RTL. The comment's claim of a symmetric overhang was false as written. A summary styled as flex is already a block-level box that fills its containing block, so dropping w-full is the whole fix; measured after at exactly 8px overhang both sides with the label still aligned to the panel content.
The small one: the file's design-token doc line still declared --foreground, which this change stopped using, and omitted the two accent tokens it now does use. That line is a maintained convention across the registry and is exactly what an agent theming a component reads first.
Also confirmed: the disabled branch cannot hover (pointer-events-none precedes any hover target), nothing in the repo asserts the old dim classes, the gitignored mirror regenerates byte-identically since this file has no lib imports, and the kit smoke suite passes with the substituted file.
* Revert "fix(ui): make the accordion trigger hover a state layer (#1137)" This reverts commit 30b9478. * revert: restore the accordion trigger's hover underline Completes the revert. The trigger is now byte-identical to what it was before either of the two hover changes, back on shadcn's underline. The reasoning I gave for replacing it (an underline reads as a link, and this is a disclosure control) was not wrong on its own terms, but it was not a problem anyone had, and shadcn parity is the stronger default for a kit whose whole promise is that shadcn knowledge transfers.
1 participant
Closes #1136
Replaces the accordion trigger's hover dim with a background state layer, the pattern Material, Carbon, and Polaris specify for disclosure headers and the kit's own most-used hover treatment (
hover:bg-accent hover:text-accent-foreground, the ghost-button and dropdown-item pair).The dim it replaces landed with #1126 and communicated with the wrong vocabulary: reduced prominence is this kit's DISABLED axis (
opacity-50, including on this very trigger), it lowered text contrast at the moment of interaction, and a text-only change cannot reveal the full-width hit area the way a tint does. Nothing has shipped in a release between the two, so this is invisible to users.Also restores rest-colour inheritance on the label (the dim needed an explicit
text-foreground; the state layer does not), and addsrounded-md -mx-2 px-2so the tint hugs the row without misaligning the label from the panel content.Test plan
.ui-preview(where--accentis re-scoped to the kit's neutral values).pointer-events-nonealready suppresses the hover.Doc surfaces
N/A beyond the in-file comment, which now records why the underline AND the dim were rejected. No markdown or docs-site page describes the trigger's hover; the ui package ships no changelog entry until its next release rolls this up.